Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backward-compatible fix for #968 #970

Merged
merged 2 commits into from
May 12, 2020
Merged

Backward-compatible fix for #968 #970

merged 2 commits into from
May 12, 2020

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Apr 28, 2020

This is @josephlr's fix (3) from #968.

Personally I see 2-5% performance penalty vs Joseph's ~16% (same benchmarks; in my case using Xeon E3-1231 aka Haswell with Fedora 31). Either way, real-world impact will usually be much lower due to the tight loops in the benchmarks.

CC @nathdobson @burdges @bjorn3.

@@ -55,8 +55,8 @@ const THREAD_RNG_RESEED_THRESHOLD: u64 = 1024 * 64;
/// [`StdRng`]: crate::rngs::StdRng
#[derive(Copy, Clone, Debug)]
pub struct ThreadRng {
// inner raw pointer implies type is neither Send nor Sync
rng: NonNull<ReseedingRng<Core, OsRng>>,
// type if neither Send nor Sync
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be is?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@dhardy dhardy merged commit f58a6b1 into rust-random:0.7 May 12, 2020
@dhardy
Copy link
Member Author

dhardy commented May 12, 2020

Darn, appears I pushed to the wrong destination and accidentally merged (no protection on this branch).

@vks do you approve or shall I revert?

@vks
Copy link
Collaborator

vks commented May 12, 2020

It's fine with me, I just did not check whether this actually fixes the use-after-free.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants